Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include chain height when running custom decision logic #322

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

arajasek
Copy link
Collaborator

@arajasek arajasek commented Jul 12, 2020

@arajasek arajasek requested a review from hannahhoward July 12, 2020 19:14
@arajasek arajasek self-assigned this Jul 12, 2020
@arajasek arajasek force-pushed the asr/seal-duration branch from a190687 to 3eb2fa9 Compare July 12, 2020 19:18
@codecov-commenter
Copy link

Codecov Report

Merging #322 into master will increase coverage by 0.06%.
The diff coverage is 16.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
+ Coverage   63.62%   63.67%   +0.06%     
==========================================
  Files          41       41              
  Lines        2608     2601       -7     
==========================================
- Hits         1659     1656       -3     
+ Misses        827      823       -4     
  Partials      122      122              
Impacted Files Coverage Δ
storagemarket/impl/provider.go 2.79% <0.00%> (-1.61%) ⬇️
...oragemarket/impl/providerstates/provider_states.go 86.90% <60.00%> (-0.91%) ⬇️
retrievalmarket/impl/blockio/traverser.go 72.23% <0.00%> (+3.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50ce1e2...3eb2fa9. Read the comment docs.

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this in general but it seems like the deal decider, which is running in the node, should have its own access to chain height, which we get from the node to begin with.

if err != nil {
return ctx.Trigger(storagemarket.ProviderEventDealRejected, xerrors.Errorf("custom deal decision logic failed to get chain head: %w", err))
}
accept, reason, err := environment.RunCustomDecisionLogic(ctx.Context(), deal, ht)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't whomever is implementing the deal decider have access to chain height on their own? We only get it from the node to begin with. My inclination would be to assume the deal decider can get access to whatever Chain APIs are needed to determine chain height.

@arajasek arajasek force-pushed the asr/seal-duration branch from 3eb2fa9 to 4383454 Compare July 15, 2020 21:38
@hannahhoward
Copy link
Collaborator

Please ignore docs-check I'm not sure why it's failing!

@hannahhoward
Copy link
Collaborator

I'm not going to rush out a new release since I think your PR no longer needs this.

@hannahhoward hannahhoward merged commit ab09b84 into master Jul 15, 2020
@arajasek arajasek deleted the asr/seal-duration branch July 16, 2020 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants